[SDAG] Introduce inbounds flag for ISD::PTRADD #162477
Conversation
This patch introduces SDNodeFlags::InBounds, to show that an ISD::PTRADD SDNode implements an inbounds getelementptr operation (i.e., the pointer operand is in bounds wrt. an allocated object it is based on, and the arithmetic does not change that). The flag is set in the DAG construction when lowering inbounds GEPs. Inbounds information is useful in the ISel when selecting memory instructions that perform address computations whose intermediate steps must be in the same memory region as the final result. Follow-up patches to propagate the flag in DAGCombines and to use it when lowering AMDGPU's flat memory instructions, where the immediate offset must not affect the memory aperture of the address (similar to this GISel patch: #153001), are planned. This mirrors #150900, which has introduced a similar flag in GlobalISel. This patch supersedes #131862, which previously attempted to introduce an SDNodeFlags::InBounds flag. The difference between this PR and #131862 is that there is now an ISD::PTRADD opcode (PR #140017) and the InBounds flag is only defined to apply to ISD::PTRADD DAG nodes. It is therefore unambiguous that in-bounds-ness refers to a memory object into which the left operand of the PTRADD node points (in contrast to #131862, where InBounds would have applied to commutative ISD::ADD nodes, so that the semantics would be more difficult to reason about). For SWDEV-516125.
ritter-x2a
added
the
llvm:SelectionDAG
Graphite App
|
@llvm/pr-subscribers-llvm-selectiondag Author: Fabian Ritter (ritter-x2a) ChangesThis patch introduces SDNodeFlags::InBounds, to show that an ISD::PTRADD SDNode Inbounds information is useful in the ISel when selecting memory instructions This mirrors #150900, which has introduced a similar flag in GlobalISel. This patch supersedes #131862, which previously attempted to introduce an For SWDEV-516125. Full diff: https://github.com/llvm/llvm-project/pull/162477.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
index 116911699ab9f..92f68ec2fb119 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -418,12 +418,16 @@ struct SDNodeFlags {
Unpredictable = 1 << 13,
// Compare instructions which may carry the samesign flag.
SameSign = 1 << 14,
+ // ISD::PTRADD operations that remain in bounds, i.e., the left operand is
+ // an address in a memory object in which the result of the operation also
+ // lies.
+ InBounds = 1 << 15,
// NOTE: Please update LargestValue in LLVM_DECLARE_ENUM_AS_BITMASK below
// the class definition when adding new flags.
PoisonGeneratingFlags = NoUnsignedWrap | NoSignedWrap | Exact | Disjoint |
- NonNeg | NoNaNs | NoInfs | SameSign,
+ NonNeg | NoNaNs | NoInfs | SameSign | InBounds,
FastMathFlags = NoNaNs | NoInfs | NoSignedZeros | AllowReciprocal |
AllowContract | ApproximateFuncs | AllowReassociation,
};
@@ -458,6 +462,7 @@ struct SDNodeFlags {
void setAllowReassociation(bool b) { setFlag<AllowReassociation>(b); }
void setNoFPExcept(bool b) { setFlag<NoFPExcept>(b); }
void setUnpredictable(bool b) { setFlag<Unpredictable>(b); }
+ void setInBounds(bool b) { setFlag<InBounds>(b); }
// These are accessors for each flag.
bool hasNoUnsignedWrap() const { return Flags & NoUnsignedWrap; }
@@ -475,6 +480,7 @@ struct SDNodeFlags {
bool hasAllowReassociation() const { return Flags & AllowReassociation; }
bool hasNoFPExcept() const { return Flags & NoFPExcept; }
bool hasUnpredictable() const { return Flags & Unpredictable; }
+ bool hasInBounds() const { return Flags & InBounds; }
bool operator==(const SDNodeFlags &Other) const {
return Flags == Other.Flags;
@@ -484,7 +490,7 @@ struct SDNodeFlags {
};
LLVM_DECLARE_ENUM_AS_BITMASK(decltype(SDNodeFlags::None),
- SDNodeFlags::SameSign);
+ SDNodeFlags::InBounds);
inline SDNodeFlags operator|(SDNodeFlags LHS, SDNodeFlags RHS) {
LHS |= RHS;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 6ea2e2708c162..68a049356033a 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -8615,7 +8615,10 @@ SDValue SelectionDAG::getMemBasePlusOffset(SDValue Ptr, SDValue Offset,
if (TLI->shouldPreservePtrArith(this->getMachineFunction().getFunction(),
BasePtrVT))
return getNode(ISD::PTRADD, DL, BasePtrVT, Ptr, Offset, Flags);
- return getNode(ISD::ADD, DL, BasePtrVT, Ptr, Offset, Flags);
+ // InBounds only applies to PTRADD, don't set it if we generate ADD.
+ SDNodeFlags AddFlags = Flags;
+ AddFlags.setInBounds(false);
+ return getNode(ISD::ADD, DL, BasePtrVT, Ptr, Offset, AddFlags);
}
/// Returns true if memcpy source is constant data.
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index c21890a0d856f..227454ae12090 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4375,6 +4375,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
if (NW.hasNoUnsignedWrap() ||
(int64_t(Offset) >= 0 && NW.hasNoUnsignedSignedWrap()))
Flags |= SDNodeFlags::NoUnsignedWrap;
+ Flags.setInBounds(NW.isInBounds());
N = DAG.getMemBasePlusOffset(
N, DAG.getConstant(Offset, dl, N.getValueType()), dl, Flags);
@@ -4418,6 +4419,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
if (NW.hasNoUnsignedWrap() ||
(Offs.isNonNegative() && NW.hasNoUnsignedSignedWrap()))
Flags.setNoUnsignedWrap(true);
+ Flags.setInBounds(NW.isInBounds());
OffsVal = DAG.getSExtOrTrunc(OffsVal, dl, N.getValueType());
@@ -4487,6 +4489,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
// pointer index type (add nuw).
SDNodeFlags AddFlags;
AddFlags.setNoUnsignedWrap(NW.hasNoUnsignedWrap());
+ AddFlags.setInBounds(NW.isInBounds());
N = DAG.getMemBasePlusOffset(N, IdxN, dl, AddFlags);
}
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
index fcfbfe6c461d3..c03420632c998 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
@@ -688,6 +688,9 @@ void SDNode::print_details(raw_ostream &OS, const SelectionDAG *G) const {
if (getFlags().hasSameSign())
OS << " samesign";
+ if (getFlags().hasInBounds())
+ OS << " inbounds";
+
if (getFlags().hasNonNeg())
OS << " nneg";
|
ritter-x2a
requested review from
antoniofrighetto,
arsenm,
davemgreen,
dtcxzyw,
efriedma-quic,
nikic,
s-barannikov and
shiltian
There was a problem hiding this comment.
Probably the header should have a warning that this is implicitly an inttoptr of the "pointer" operand, so the set of optimizations that apply is restricted. As we discussed in #131862, a lot of obvious optimizations don't work because it's hard to prove anything about the provenance of the pointer value.
Otherwise, I think this is reasonable.
|
(I'm not disagreeing with the point and will add a corresponding warning comment; this is just for me to better understand the problem.) Could you give an example of an optimization that doesn't work? Would for example a and the inbounds-ness of the outer ptradd / the last gep hinges on the |
| SameSign = 1 << 14, | ||
| // ISD::PTRADD operations that remain in bounds, i.e., the left operand is | ||
| // an address in a memory object in which the result of the operation also | ||
| // lies. WARNING: Since SDAG does not have pointer types, a PTRADD's pointer |
There was a problem hiding this comment.
Nit: As part of the CHERI upstreaming work we do now have a c64/c128 type which is actually a pointer type.
| // lies. WARNING: Since SDAG does not have pointer types, a PTRADD's pointer | |
| // lies. WARNING: Since SDAG generally uses integers instead of pointer types, a PTRADD's pointer |
Slightly more realistically, you'd have pointers to objects that are adjacent, or have non-overlapping lifetimes, that happen to compare equal, and you swap one integer value for the other. But that's the basic idea, yes. |
4 participants
This patch introduces SDNodeFlags::InBounds, to show that an ISD::PTRADD SDNode
implements an inbounds getelementptr operation (i.e., the pointer operand is in
bounds wrt. an allocated object it is based on, and the arithmetic does not
change that). The flag is set in the DAG construction when lowering inbounds
GEPs.
Inbounds information is useful in the ISel when selecting memory instructions
that perform address computations whose intermediate steps must be in the same
memory region as the final result. Follow-up patches to propagate the flag in
DAGCombines and to use it when lowering AMDGPU's flat memory instructions,
where the immediate offset must not affect the memory aperture of the address
(similar to this GISel patch: #153001), are planned.
This mirrors #150900, which has introduced a similar flag in GlobalISel.
This patch supersedes #131862, which previously attempted to introduce an
SDNodeFlags::InBounds flag. The difference between this PR and #131862 is that
there is now an ISD::PTRADD opcode (PR #140017) and the InBounds flag is only
defined to apply to ISD::PTRADD DAG nodes. It is therefore unambiguous that
in-bounds-ness refers to a memory object into which the left operand of the
PTRADD node points (in contrast to #131862, where InBounds would have applied
to commutative ISD::ADD nodes, so that the semantics would be more difficult to
reason about).
For SWDEV-516125.